fix(core): reject a DELETE or an UPDATE whose WHERE clause cannot reach the provider - #24657
Open
michaelsembwever wants to merge 1 commit into
Open
Conversation
…ch the provider A `DELETE` or an `UPDATE` whose `WHERE` clause holds an `IN` or an `EXISTS` subquery changed every row of the target table, and reported the whole table as affected. The optimizer rewrites the subquery into a semi join, so the condition leaves the `Filter` nodes that `extract_dml_filters()` reads. The provider then received an empty filter list, which is the encoding for "no WHERE clause", and applied the statement to all rows. An always-false `WHERE` clause reached the provider the same way. The simplifier folds the predicate into an empty relation, so again no filter survived, and a `DELETE FROM t WHERE false` emptied the table. Add `classify_dml_input()`, which walks the input plan of a `DELETE` or an `UPDATE` before the provider hook runs: - an empty relation means that no row matches, so the statement reports a count of 0 and the hook is not called; - a join, a predicate on another table, or any other node that restricts or multiplies rows raises a "not implemented" error, and the hook is not called. The hook stays untouched in every rejected case, so a provider that writes to durable storage cannot lose rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
core
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24657 +/- ##
==========================================
+ Coverage 81.36% 81.44% +0.07%
==========================================
Files 1117 1118 +1
Lines 397872 399611 +1739
Branches 397872 399611 +1739
==========================================
+ Hits 323725 325451 +1726
+ Misses 55229 55153 -76
- Partials 18918 19007 +89 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
martin-g
reviewed
Aug 27, 2026
Comment on lines
+2255
to
+2257
| /// [`DmlInput::Filters`] when the provider may be called, [`DmlInput::NoRows`] | ||
| /// when the statement matches no row, and a "not implemented" error when part of | ||
| /// the `WHERE` clause cannot reach the provider. |
Member
There was a problem hiding this comment.
as bullets
Suggested change
| /// [`DmlInput::Filters`] when the provider may be called, [`DmlInput::NoRows`] | |
| /// when the statement matches no row, and a "not implemented" error when part of | |
| /// the `WHERE` clause cannot reach the provider. | |
| /// * [`DmlInput::Filters`] when the provider may be called | |
| /// * [`DmlInput::NoRows`] when the statement matches no row | |
| /// * a "not implemented" error when part of the `WHERE` clause cannot reach the provider. |
Comment on lines
+2263
to
+2272
| let mut allowed_refs = vec![target.clone()]; | ||
| input.apply(|node| { | ||
| if let LogicalPlan::SubqueryAlias(alias) = node | ||
| && let LogicalPlan::TableScan(scan) = alias.input.as_ref() | ||
| && scan.table_name.resolved_eq(target) | ||
| { | ||
| allowed_refs.push(TableReference::bare(alias.alias.to_string())); | ||
| } | ||
| Ok(TreeNodeRecursion::Continue) | ||
| })?; |
Member
There was a problem hiding this comment.
This duplicates the same logic at https://github.com/thelastpickle/datafusion/blob/5f3e301801e3c27f3a3dd69974f71d97e811faad/datafusion/core/src/physical_planner.rs#L2376C5-L2388
It would be good to extract it to a helper function and reuse it.
| CREATE TABLE test_delete_subquery_src AS VALUES (2); | ||
|
|
||
| statement error DataFusion error: This feature is not implemented: DELETE on table 'test_delete_subquery' with an IN or an EXISTS subquery in its WHERE clause is not supported | ||
| DELETE FROM test_delete_subquery WHERE column1 IN (SELECT column1 FROM test_delete_subquery_src); |
Member
There was a problem hiding this comment.
It would be good to execute the tests also with set datafusion.optimizer.max_passes = 0;.
Some other tests exercise this.
| | LogicalPlan::Sort(_) | ||
| | LogicalPlan::Repartition(_) | ||
| // A `Limit` reaches the provider as no filter at all, so a DELETE | ||
| // ignores it. That is a separate gap, kept as it is here. |
Member
There was a problem hiding this comment.
let's file an issue for this. I didn't find an existing one
| zero_rows_affected_exec(Arc::clone(output_schema.inner()))? | ||
| } | ||
| DmlInput::Filters => { | ||
| let filters = extract_dml_filters(input, table_name)?; |
Member
There was a problem hiding this comment.
nit: both classify_dml_input() and extract_dml_filters() traverse the tree to collect the allowed_refs. Is it worthy to collect them before calling these methods ?! It looks negligible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
#24656
Rationale for this change
See ticket.
What changes are included in this PR?
A
DELETEor anUPDATEwhoseWHEREclause holds anINor anEXISTSsubquery changed every row of the target table, and reported the whole table as affected. The optimizer rewrites the subquery into a semi join, so the condition leaves theFilternodes thatextract_dml_filters()reads. The provider then received an empty filter list, which is the encoding for "no WHERE clause", and applied the statement to all rows.An always-false
WHEREclause reached the provider the same way. The simplifier folds the predicate into an empty relation, so again no filter survived, and aDELETE FROM t WHERE falseemptied the table.Add
classify_dml_input(), which walks the input plan of aDELETEor anUPDATEbefore the provider hook runs:The hook stays untouched in every rejected case, so a provider that writes to durable storage cannot lose rows.
Are these changes tested?
Only with the tests provided in this patch, which are based on the assumptions made in the ticket description.
Are there any user-facing changes?
?